-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ui] For runs targeting assets, show >1 asset in the Target column #26122
Conversation
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit 83bebea. |
cefdda4
to
5d19ed5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better. Love this
We’ve had reports that users who launch small N numbers of assets together find the new run feed UI difficult to use, since it shows either “asset name” or “2 assets” in the Target column. This PR: - Expands the target column a bit - Shows as many asset / check tags as will fit in the available space, and then “X more” The implementation of this uses a new hook `useAdjustChildVisibilityToFill`. The idea is that your component renders some reasonable max number of tags (10) and a more tag, and the hook uses a resize observer + layout effect to show/hide the tags to fit the available space. I tried doing this using React state, but it looks bad if you can see it adding / removing tags. I think the other approach would be to write a tag “measure” function, or otherwise repeatedly render + size them offscreen, but that’d still force layouts, and in this case the tags are not identical react components (the “4 more” tag is slightly different) I added a storybook that makes it easy to test what this looks like in a bunch of scenarios. Sidenote: There’s a bunch of cruft here because the “Target” column components all have to support a “tags” rendering and a “plain” rendering. This is going away soon when we remove the FF allowing users to revert to the old runs page, and I think it’ll clean up this code!
5d19ed5
to
83bebea
Compare
…agster-io#26122) We’ve had reports that users who launch small N numbers of assets together find the new run feed UI difficult to use, since it shows either “asset name” or “2 assets” in the Target column. The "2 assets" form makes it difficult to scan for a run you have in mind: <img width="1005" alt="image" src="https://github.com/user-attachments/assets/fa2dcd55-5ae9-47a4-b402-e54fc67ec396"> This PR: - Makes the target column a bit wider - Shows as many asset / check tags as will fit in the available space, and then “X more” The implementation of this uses a new hook `useAdjustChildVisibilityToFill`. The idea is that your component renders some reasonable max number of tags (10) and a more tag, and the hook uses a resize observer + layout effect to show/hide the tags to fit the available space. I tried doing this using React state, but it looks bad because you can see it adding / removing tags, especially as you resize the viewport. I think the other approach would be to write a tag “measure” function, or otherwise repeatedly render + size them offscreen, but that’d still force layouts with the added cost of a React.render (the tags are not identical react components - the “4 more” tag is slightly different - so I think the offscreen approach would need to actually render the tag...) The text + size of the "more" tag also changes based on the number of tags displayed. Sidenote: There’s a bunch of cruft here because the “Target” column components all have to support a “tags” rendering and a “plain” rendering. This is going away soon when we remove the FF allowing users to revert to the old runs page, and I think it’ll clean up this code! ## How I Tested These Changes I added a storybook that makes it easy to test what this looks like in a bunch of scenarios and verify it works nicely. https://github.com/user-attachments/assets/8dc2afdb-e15c-4fab-a139-eb5698428bf1 Before: <img width="1191" alt="image" src="https://github.com/user-attachments/assets/cbd74932-1b22-4ffa-9c03-9375980cf1d9"> <img width="1186" alt="image" src="https://github.com/user-attachments/assets/926a1075-4aac-404e-9952-798df82d8184"> After: <img width="1189" alt="image" src="https://github.com/user-attachments/assets/7e33c045-43ba-4370-a03f-a4c505b3606b"> <img width="1183" alt="image" src="https://github.com/user-attachments/assets/6e81a1d8-1a91-4b5e-b87f-dfdbb01b102e"> Related: https://linear.app/dagster-labs/issue/FE-702/show-more-than-one-asset-tag-on-the-new-runs-feed-page Co-authored-by: bengotow <[email protected]>
We discussed this previously in #26122 — using a `useEffect + requestAnimationFrame` is technically the ideal time to perform the DOM sizing code in the asset tag collections, but it performs really badly on the runs page. This is because the tag collection appears within each row of a virtualized table, and the `useEffect + requestAnimationFrame` combo causes our layout to happen _after_ the rows have been measured, and the changes cause them to be re-measured.
|
||
evaluatingRef.current = true; | ||
|
||
let count = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should count start at 1?
We discussed this previously in #26122 — using a `useEffect + requestAnimationFrame` is technically the ideal time to perform the DOM sizing code in the asset tag collections, but it performs really badly on the runs page. This is because the tag collection appears within each row of a virtualized table, and the `useEffect + requestAnimationFrame` combo causes our layout to happen _after_ the rows have been measured, and the changes cause them to be re-measured.
## Summary & Motivation We discussed this previously in #26122 — using a `useEffect + requestAnimationFrame` is technically the ideal time to perform the DOM sizing code in the asset tag collections, but it performs badly on the runs page due to interactions with react-virtual. The tag collection appears within each row of a virtualized table, and I think that the `useEffect + requestAnimationFrame` combo causes our layout to happen _after_ the rows have been measured, and the height changes cause them all to be re-measured after a visible re-paint. ## How I Tested These Changes The bad traces look like this: <img width="1266" alt="image" src="https://github.com/user-attachments/assets/c5cc0837-5019-4093-80e3-dd2b2c5362fd" /> After: (note this is a key-up event causing a big scroll to lots of new rows all at once) <img width="1244" alt="image" src="https://github.com/user-attachments/assets/036c4b16-2590-42ab-be79-a3bd2a72a118" /> --------- Co-authored-by: bengotow <[email protected]>
We’ve had reports that users who launch small N numbers of assets together find the new run feed UI difficult to use, since it shows either “asset name” or “2 assets” in the Target column. The "2 assets" form makes it difficult to scan for a run you have in mind:
This PR:
The implementation of this uses a new hook
useAdjustChildVisibilityToFill
. The idea is that your component renders some reasonable max number of tags (10) and a more tag, and the hook uses a resize observer + layout effect to show/hide the tags to fit the available space. I tried doing this using React state, but it looks bad because you can see it adding / removing tags, especially as you resize the viewport. I think the other approach would be to write a tag “measure” function, or otherwise repeatedly render + size them offscreen, but that’d still force layouts with the added cost of a React.render (the tags are not identical react components - the “4 more” tag is slightly different - so I think the offscreen approach would need to actually render the tag...) The text + size of the "more" tag also changes based on the number of tags displayed.Sidenote: There’s a bunch of cruft here because the “Target” column components all have to support a “tags” rendering and a “plain” rendering. This is going away soon when we remove the FF allowing users to revert to the old runs page, and I think it’ll clean up this code!
How I Tested These Changes
I added a storybook that makes it easy to test what this looks like in a bunch of scenarios and verify it works nicely.
Screen.Recording.2024-11-24.at.12.21.15.PM.mov
Before:
After:
Related: https://linear.app/dagster-labs/issue/FE-702/show-more-than-one-asset-tag-on-the-new-runs-feed-page